Skip to content

HA2#81

Open
Aurielle-t wants to merge 6 commits intoProfWider:masterfrom
Aurielle-t:master
Open

HA2#81
Aurielle-t wants to merge 6 commits intoProfWider:masterfrom
Aurielle-t:master

Conversation

@Aurielle-t
Copy link

No description provided.

@ProfWider ProfWider self-requested a review October 31, 2020 22:44
@ProfWider ProfWider self-assigned this Oct 31, 2020
Copy link
Owner

@ProfWider ProfWider left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sie sind spät dran! Zwei Dinge: die Commits sollten aufeinander aufbauen, d.h. im letzten Commit sollten 4 Tests und zwei Bugfixes gleichzeitig im Code sein und alle zusammen miteinander funktionieren. Soweit ich das überblicken kann widersprechen sich Ihre Bugfixes nicht, insofern ok. Das andere: eigentlich müssten Sie noch mindestens eine der funktionen pressClearKey, pressDotKey, oder pressNegative testen (habe ich zumindest auf Slack so formuliert). Aber auch hier ok, auf Moodle steht das nicht ganz explizit so. Also: gerade so bestanden! Aber stellen Sie bitte doch ruhig nochmal einen neuen PR (also nochmal neu forken, klonen, etc.), wo die Commits aufeinander aufbauen. Ist aber jetzt nicht schlimm, wenn das nicht mehr vor Mitternacht ist.

Copy link
Owner

@ProfWider ProfWider left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Achja, Ihr letzter Bugfix ist auch wirklich abenteuerlich - es ist zwar super, dass Sie die Division durch 0 abgedeckt haben, aber mit dem aktuellen Bugfix würde der Calculator-Screen bei 5 minus 4 auch Error anzeigen, da hätten Sie lieber so etwas wie NaN (not a number) nehmen können, um diesen Fall zu kommunizieren. Aber gut, auch hier, die Tests, die da sind, sind grün, also reicht das aus. Und ich freu mich, dass Sie es (gerade so) noch geschafft haben.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants